Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get elements for IDs passed as contentEl and ptrEl options. Fixes #11. #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raglan-road
Copy link

No description provided.

@raglan-road
Copy link
Author

Uh oh...I've botched this pull request and it's not attached to #11. Sorry!

@mk-pmb
Copy link

mk-pmb commented Nov 9, 2017

Looks like with this PR it will accept strings only? In that case it would break for people who use the work-around of supplying the element.

@raglan-road
Copy link
Author

So this is from a couple years ago, but I think I remember why I created this pull request.

The documentation in the code says these options are strings representing the IDs of the elements involved in the pull-to-refresh action. However, in the code they're not used as such: they're used as plain old elements (or whatever you pass in). This pull request was meant to close that loophole: if we're supposed to be passing in IDs as strings, we should use those strings to get the proper elements by ID. But, fixing the parameters to match the documentation would break what you're trying to do (pass in an element directly).

Since this pull request is two years old and the repo itself hasn't been updated in three, I'd say it's probably a dead issue, but it depends on whether or not you want the options to match what they're documented to be. They could just as easily be of any type with type checking to see if they're a string or a DOMElement and then react appropriately based on what was discovered. YMMV!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants